Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add test for inserting anchor without tx #961

Closed
wants to merge 4 commits into from
Closed

Add test for inserting anchor without tx #961

wants to merge 4 commits into from

Conversation

remix7531
Copy link
Contributor

Description

This adds a test case described in issue958

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

evanlinjin
evanlinjin requested changes May 3, 2023
View reviewed changes
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work so far. Remember that we also want to test recovering the appended additions into a new graph. Refer to #958 (comment)

crates/chain/tests/test_tx_graph.rs Outdated Show resolved Hide resolved
crates/chain/tests/test_tx_graph.rs Outdated Show resolved Hide resolved
@remix7531 remix7531 requested a review from evanlinjin May 8, 2023 12:58
@remix7531 remix7531 force-pushed the issue958 branch 2 times, most recently from e4bdd6d to df0b541 Compare May 9, 2023 13:12
evanlinjin
evanlinjin requested changes May 9, 2023
View reviewed changes
Copy link
Member

@evanlinjin evanlinjin left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work so far. Few notes.

  1. Ensure CI passes.
  2. Ensure GitHub can verify your commits (Maybe you need to give your PGP or SSH public key to GitHub?).
  3. Please rebase your commits into one.
  4. Use loops where you can (for example if you are inserting multiple anchors or multiple txs).

@remix7531 remix7531 requested a review from evanlinjin May 15, 2023 11:16
danielabrozzoni
danielabrozzoni reviewed May 16, 2023
View reviewed changes
Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few improvements can be made:

  • Fix the warnings cargo clippy is throwing
  • Use a more descriptive commit message. Instead of "tests for issue958", describe which test you're adding in the title, and add "Partially fixes More tests for IndexedTxGraph and TxGraph #958" in the description. In general, it should be possible to understand what a commit is doing from the title/description without having to open github

The rest looks good to me.

@remix7531
Adds test inserting anchor without tx
0536bd5 
Test inserting anchors without the corresponding transaction
and testing the recoverability of the transaction graph

Partially fixes "More tests for IndexedTxGraph and TxGraph bitcoindevkit#958"
bitcoindevkit#958
evanlinjin
evanlinjin requested changes May 17, 2023
View reviewed changes
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you again, sorry I think it will be better to reduce the scope of this test.

crates/chain/tests/test_tx_graph.rs Outdated Show resolved Hide resolved
crates/chain/tests/test_tx_graph.rs Outdated Show resolved Hide resolved
@notmandatory notmandatory added this to the 1.0.0-alpha.1 milestone May 23, 2023
@remix7531 remix7531 requested a review from evanlinjin May 23, 2023 15:46
@remix7531 remix7531 requested a review from evanlinjin June 5, 2023 12:57
@notmandatory
Copy link
Member

Since it looks like still a bit of work today I'm moving this out of the alpha.1 milestone.

@notmandatory notmandatory removed this from the 1.0.0-alpha.1 milestone Jun 19, 2023
@notmandatory notmandatory added this to the 1.0.0-alpha.2 milestone Jun 20, 2023
@remix7531
Copy link
Contributor Author

I do not understand what is missing here. The workflows failed because of this
error: package log v0.4.19 cannot be built because it requires rustc 1.60.0 or newer, while the currently active rustc version is 1.57.0

Just tested the workflows locally and it seams to work now.

@notmandatory notmandatory mentioned this pull request Jul 15, 2023
Closed
19 tasks
@danielabrozzoni
Copy link
Member

Hey @remix7531, sorry for the late response! I think you encountered a problem with our CI, that's why it would work locally but fail here. Can you try rebasing?

@oleonardolima
Copy link
Contributor

Is this something still needed for 1.0.0-beta milestone ? (cc @notmandatory @ValuedMammal)

I got a bit confused because I noticed that the #958 issue already has been closed but without the tests covered by this PR.

@ValuedMammal
Copy link
Contributor

I think it wouldn't hurt to have a test similar to this

@ValuedMammal ValuedMammal mentioned this pull request Nov 20, 2024
Closed
@notmandatory
Copy link
Member

This testing could help with verifying sqlite schema question in #1712

@notmandatory notmandatory mentioned this pull request Nov 21, 2024
Merged
8 tasks
evanlinjin added a commit that referenced this pull request Nov 28, 2024
@evanlinjin
Merge #1736: Sqlite - allow persisting anchor without tx
bf42562 
18f5f3f test(sqlite): test persisting anchors and txs independently (valued mammal)
297ff34 test(chain): add test `insert_anchor_without_tx` (valued mammal)
e69d10e doc(chain): document module `rusqlite_impl` (valued mammal)
8fa899b fix(chain): Sqlite - allow persisting anchor without tx (志宇)

Pull request description:

  ### Description

  Previously, we may error when we insert an anchor where the txid being anchored has no corresponding tx.

  closes #1712
  replaces #961

  ### Notes to the reviewers

  <!-- In this section you can include notes directed to the reviewers, like explaining why some parts
  of the PR were done in a specific way -->

  ### Changelog notice

  <!-- Notice the release manager should include in the release tag message changelog -->
  <!-- See https://keepachangelog.com/en/1.0.0/ for examples -->

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

  #### New Features:

  * [ ] I've added tests for the new feature
  * [ ] I've added docs for the new feature

  #### Bugfixes:

  * [ ] This pull request breaks the existing API
  * [x] I've added tests to reproduce the issue which are now passing
  * [x] I'm linking the issue being fixed by this PR

ACKs for top commit:
  evanlinjin:
    ACK 18f5f3f
  ValuedMammal:
    ACK 18f5f3f

Tree-SHA512: 7343eff857016f684cfb9f7af57f7be56ba36c70c36b8b4303159636f79ad5cc49a6f94354443323c9aa05c31ec7d43c22b038d9e41361596c3a346bd6a94b10
@ValuedMammal ValuedMammal removed this from the 1.0.0-beta milestone Dec 1, 2024
@evanlinjin
Copy link
Member

Tests for inserting anchor without tx already exist:

bdk/crates/chain/tests/test_tx_graph.rs

Lines 1074 to 1102 in f6c1c61

fn insert_anchor_without_tx() {
let mut graph = TxGraph::<BlockId>::default();
let tx = new_tx(21);
let txid = tx.compute_txid();
let anchor = BlockId {
height: 100,
hash: hash!("A"),
};
// insert anchor with no corresponding tx
let mut changeset = graph.insert_anchor(txid, anchor);
assert!(changeset.anchors.contains(&(anchor, txid)));
// recover from changeset
let mut recovered = TxGraph::default();
recovered.apply_changeset(changeset.clone());
assert_eq!(recovered, graph);
// now insert tx
let tx = Arc::new(tx);
let graph_changeset = graph.insert_tx(tx.clone());
assert!(graph_changeset.txs.contains(&tx));
changeset.merge(graph_changeset);
// recover from changeset again
let mut recovered = TxGraph::default();
recovered.apply_changeset(changeset);
assert_eq!(recovered, graph);
}

@evanlinjin evanlinjin closed this Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evanlinjin evanlinjin Awaiting requested review from evanlinjin

@danielabrozzoni danielabrozzoni Awaiting requested review from danielabrozzoni

Assignees

@remix7531 remix7531

Labels
module-blockchain tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@remix7531 @notmandatory @danielabrozzoni @oleonardolima @ValuedMammal @evanlinjin @nondiremanuel